Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

tests/unit/crossProjectIndexService.test.ts (1)

48-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated raw IndexedDB test helpers leave connections open. Both new test files add their own open/getAll/put wrappers over raw IndexedDB and never close the returned IDBDatabase. One shared helper removes the duplication and gives a single place to close handles.

• tests/unit/crossProjectIndexService.test.ts#L48-L76: replace readRawIndex and writeRawIndex with calls into the shared helper and close each connection after use.
• tests/unit/aiInferenceCacheEncryption.test.ts#L14-L42: replace readRawEntries and writeRawEntry with the same shared helper, and import the database and store names from the cache service instead of redeclaring DB_NAME and STORE.

As per coding guidelines: "Apply DRY: place reusable logic in services, hooks, or feature thunks instead of duplicating it in views."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/crossProjectIndexService.test.ts` around lines 48 - 76, Replace
the duplicated raw IndexedDB helpers in
tests/unit/crossProjectIndexService.test.ts#L48-L76 and
tests/unit/aiInferenceCacheEncryption.test.ts#L14-L42 with one shared test
helper that opens, operates on, and closes the database connection; update both
test files to use it, and in
tests/unit/aiInferenceCacheEncryption.test.ts#L14-L42 import the database and
store names from the cache service instead of redeclaring DB_NAME and STORE.

Source: Coding guidelines

services/crossProjectIndexService.ts (2)

88-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add QNBS-v3 why-comments for the new encode and migrate helpers.

encodeProjectSearchIndex and the migration write inside enrichProjectIndex are non-trivial changes without a QNBS-v3 comment. indexProject at line 176 and listIndexedProjects at line 217 already carry one. Add one single-line comment for each new behaviour, in particular why enrichProjectIndex persists a migrated record at line 267 before the enriched write at line 292.

As per coding guidelines: "For every non-trivial code change, add one single-line QNBS-v3 comment explaining why, using the appropriate TS/JS, JSX, or CSS syntax; never wrap the comment across physical lines."

Also applies to: 246-268

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/crossProjectIndexService.ts` around lines 88 - 97, Add single-line
QNBS-v3 comments explaining the rationale for the new behavior in
encodeProjectSearchIndex and the migration write within enrichProjectIndex. In
particular, document why the migrated record is persisted before the later
enriched write; keep each comment on one physical line and use valid TypeScript
comment syntax.

Source: Coding guidelines

---

205-226: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

One corrupt record makes the whole project list unreadable.

listIndexedProjects decodes sequentially and lets the first SecureRecordCorruptError reject the whole call. Cross-project search then fails completely, even when only one project record is damaged. Fail-closed is correct for locked storage, because the key is missing for every record. A single corrupt record is different: the remaining records are still recoverable.

Consider collecting corrupt projectId values, logging them through services/logger.ts with warn, and returning the decodable records. Keep the locked error propagating.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/crossProjectIndexService.ts` around lines 205 - 226, Update
listIndexedProjects to handle SecureRecordCorruptError per record: collect the
affected projectId values, log them with the services/logger.ts warn facility,
and continue decoding the remaining records. Preserve propagation of
locked-storage/key errors, and keep migration and sorting behavior for
successfully decoded records unchanged.

services/ai/aiInferenceCacheService.ts (1)

186-197: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Note the divergence between memory and persistent cache on migration and eviction.

When this.db is null the entry only enters memory, and when encryption is locked nothing is cached at all. Both paths are intentional. One residual gap: the in-memory map keeps the plaintext result after a later clearIdbEncryptionKey() in the same session, so a locked state still serves cached results from memory. If the lock is meant to stop all reads, clear inMemory when the key is cleared.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/ai/aiInferenceCacheService.ts` around lines 186 - 197, Update
clearIdbEncryptionKey to clear the inMemory cache when the encryption key is
cleared, ensuring subsequent locked-state reads cannot serve previously cached
plaintext results while preserving the existing persistent-cache handling.

tests/unit/aiInferenceCacheEncryption.test.ts (1)

14-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Share the store constants instead of duplicating the literals.

DB_NAME and STORE repeat values owned by services/ai/aiInferenceCacheService.ts. If the service renames its database or store, readRawEntries opens a fresh empty database and the assertions fail with a confusing "expected defined" message rather than a rename signal. Export the constants from the service module and import them here.

The two helpers also leave each opened IDBDatabase open. Call db.close() after the read or write so a version change in a later test is not blocked.

Also applies to: 17-42

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/aiInferenceCacheEncryption.test.ts` around lines 14 - 15, Export
the database and store constants from aiInferenceCacheService and import them
into the tests instead of redeclaring literals in
aiInferenceCacheEncryption.test.ts. Update both helper functions, readRawEntries
and the write helper, to call db.close() after completing their IndexedDB read
or write operations, including the relevant cleanup path.

services/storage/storageEncryptionService.ts (1)

36-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the required single-line QNBS-v3 why-comments.

• services/storage/storageEncryptionService.ts#L36-L68: add one comment that explains the versioned envelope and fail-closed storage contract.
• services/storage/idbPassphraseSentinel.ts#L46-L51: add one comment that explains why test resets must close cached IDB handles.
• tests/unit/storage/storageEncryptionService.test.ts#L177-L267: add one comment that explains the secure-record regression coverage.

As per coding guidelines, “For every non-trivial code change, add one single-line QNBS-v3 comment explaining why.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/storage/storageEncryptionService.ts` around lines 36 - 68, Add one
single-line QNBS-v3 why-comment in services/storage/storageEncryptionService.ts
near SecureRecordEnvelope and the related errors, explaining the versioned
envelope and fail-closed storage contract. Add one single-line QNBS-v3
why-comment in services/storage/idbPassphraseSentinel.ts near the test-reset
logic, explaining why cached IndexedDB handles must be closed. Add one
single-line QNBS-v3 why-comment in
tests/unit/storage/storageEncryptionService.test.ts covering the secure-record
regression tests, explaining their purpose.

Source: Coding guidelines

services/sceneRevisionService.ts (1)

82-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Collapse the duplicated revision assembly.

Both branches build the same SceneRevision from id, sectionId, createdAt, and the decoded payload. Only the payload source differs. Select the payload source first, then assemble once.

♻️ Proposed refactor

 async function decodeRevision(
   stored: unknown,
 ): Promise<{ revision: SceneRevision; needsMigration: boolean }> {
-  if (isStoredSceneRevision(stored)) {
-    const decoded = await readSecureRecordPayload<SceneRevisionPayload>(stored.payload);
-    return {
-      revision: {
-        id: stored.id,
-        sectionId: stored.sectionId,
-        createdAt: stored.createdAt,
-        ...decoded.value,
-      },
-      needsMigration: decoded.needsMigration,
-    };
-  }
-
-  const legacy = stored as SceneRevision;
-  const decoded = await readSecureRecordPayload<SceneRevisionPayload>(revisionPayload(legacy));
+  const meta = stored as StoredSceneRevision | SceneRevision;
+  const rawPayload = isStoredSceneRevision(stored)
+    ? stored.payload
+    : revisionPayload(stored as SceneRevision);
+  const decoded = await readSecureRecordPayload<SceneRevisionPayload>(rawPayload);
   return {
     revision: {
-      id: legacy.id,
-      sectionId: legacy.sectionId,
-      createdAt: legacy.createdAt,
+      id: meta.id,
+      sectionId: meta.sectionId,
+      createdAt: meta.createdAt,
       ...decoded.value,
     },
     needsMigration: decoded.needsMigration,
   };
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/sceneRevisionService.ts` around lines 82 - 109, Refactor
decodeRevision to select the payload source based on
isStoredSceneRevision(stored), preserving the stored and legacy payload paths,
then call readSecureRecordPayload once and assemble the SceneRevision once from
the appropriate record’s id, sectionId, createdAt, and decoded value. Preserve
the existing needsMigration result.

services/proForge/proForgeMemoryBank.ts (1)

106-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the duplicated entry-assembly in both branches.

Both branches build the same MemoryBankEntry shape from id, projectId, category, createdAt, and the decoded payload. Only the payload source differs. Extract the payload source first, then assemble once.

♻️ Proposed refactor

 async function decodeMemoryEntry(
   stored: StoredMemoryEntry | MemoryBankEntry,
 ): Promise<{ entry: MemoryBankEntry; needsMigration: boolean }> {
-  if ('payload' in stored) {
-    const decoded = await readSecureRecordPayload<MemoryEntryPayload>(stored.payload);
-    return {
-      entry: {
-        id: stored.id,
-        projectId: stored.projectId,
-        category: stored.category,
-        createdAt: stored.createdAt,
-        ...decoded.value,
-      },
-      needsMigration: decoded.needsMigration,
-    };
-  }
-
-  const decoded = await readSecureRecordPayload<MemoryEntryPayload>(memoryPayload(stored));
+  const rawPayload = 'payload' in stored ? stored.payload : memoryPayload(stored);
+  const decoded = await readSecureRecordPayload<MemoryEntryPayload>(rawPayload);
   return {
     entry: {
       id: stored.id,
       projectId: stored.projectId,
       category: stored.category,
       createdAt: stored.createdAt,
       ...decoded.value,
     },
     needsMigration: decoded.needsMigration,
   };
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/proForge/proForgeMemoryBank.ts` around lines 106 - 134, Refactor
decodeMemoryEntry so it selects the payload source first—stored.payload for
records containing payload, otherwise memoryPayload(stored)—and calls
readSecureRecordPayload once. Assemble the MemoryBankEntry and return
needsMigration once from the shared decoded result, preserving the existing
fields and migration behavior.

services/proForge/proForgeHistoryStore.ts (1)

106-108: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Lazy-migration writes are awaited inside the read path in all three stores. Each read decodes the payload successfully and then awaits a rewrite of the migrated record. If the migration transaction fails, the read rejects and the caller loses data it could already display. Decouple the migration write from the read result in each store.

• services/proForge/proForgeHistoryStore.ts#L106-L108: wrap the putHistory migration call in a try/catch, log the failure through services/logger.ts, and still return decoded.value.runs.
• services/proForge/proForgeMemoryBank.ts#L196-L201: wrap the putMemoryEntries(db, migrations) call in a try/catch, log the failure, and still return entries.
• services/sceneRevisionService.ts#L177-L183: wrap the putStoredRevisions(db, migrations) call in a try/catch, log the failure, and still return the sorted revisions.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/proForge/proForgeHistoryStore.ts` around lines 106 - 108, Decouple
lazy-migration writes from successful reads by wrapping the migration
persistence calls in try/catch blocks and logging failures through
services/logger.ts. In services/proForge/proForgeHistoryStore.ts lines 106-108,
preserve returning decoded.value.runs; in
services/proForge/proForgeMemoryBank.ts lines 196-201, preserve returning
entries; and in services/sceneRevisionService.ts lines 177-183, preserve
returning the sorted revisions even when putHistory, putMemoryEntries, or
putStoredRevisions fails.

tests/unit/proForge/proForgeHistoryStore.test.ts (1)

25-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Close the raw IndexedDB connections in the test helpers.

readRawHistory and writeRawHistory open a new connection on every call and never close it. Open connections block versionchange and database deletion, so a later test can hang or fail if the schema changes. Close the connection after the request settles. The same pattern exists in tests/unit/proForge/proForgeMemoryBank.test.ts and tests/unit/sceneRevisionService.test.ts.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/proForge/proForgeHistoryStore.test.ts` around lines 25 - 50,
Update the readRawHistory and writeRawHistory test helpers to close each
IndexedDB connection after its read or write transaction settles, including
error paths. Apply the same connection-cleanup pattern to the corresponding raw
IndexedDB helpers in proForgeMemoryBank.test.ts and
sceneRevisionService.test.ts.

tests/unit/lora/loraAdapterEncryption.test.ts (1)

76-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Do not pin the raw helpers to database version 2, and close the connections.

readRaw and writeRaw hardcode version 2. The service owns DB_VERSION. If that constant increases, both helpers request a lower version and fail with VersionError, which produces a confusing test failure instead of a schema signal. Omit the version so the helpers open the current database.

Both helpers also leave the connection open. A leaked connection blocks a later versionchange upgrade inside the same test.

♻️ Proposed fix

 async function readRaw(
   storeName: string,
   key: string,
 ): Promise<Record<string, unknown> | undefined> {
+  // QNBS-v3: Open without a version so the helper tracks the service schema instead of pinning it.
   const db = await new Promise<IDBDatabase>((resolve, reject) => {
-    const request = indexedDB.open(DB_NAME, 2);
+    const request = indexedDB.open(DB_NAME);
     request.onsuccess = () => resolve(request.result);
     request.onerror = () => reject(request.error);
   });
-  return new Promise((resolve, reject) => {
+  const result = await new Promise<Record<string, unknown> | undefined>((resolve, reject) => {
     const request = db.transaction(storeName, 'readonly').objectStore(storeName).get(key);
     request.onsuccess = () => resolve(request.result as Record<string, unknown> | undefined);
     request.onerror = () => reject(request.error);
   });
+  db.close();
+  return result;
 }
 
 async function writeRaw(storeName: string, record: Record<string, unknown>): Promise<void> {
   const db = await new Promise<IDBDatabase>((resolve, reject) => {
-    const request = indexedDB.open(DB_NAME, 2);
+    const request = indexedDB.open(DB_NAME);
     request.onsuccess = () => resolve(request.result);
     request.onerror = () => reject(request.error);
   });
   await new Promise<void>((resolve, reject) => {
     const transaction = db.transaction(storeName, 'readwrite');
     transaction.objectStore(storeName).put(record);
     transaction.oncomplete = () => resolve();
     transaction.onerror = () => reject(transaction.error);
   });
+  db.close();
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/lora/loraAdapterEncryption.test.ts` around lines 76 - 104, Update
the IndexedDB opening logic in readRaw and writeRaw to omit the hardcoded
version argument, allowing the current database version to be used, and close
each resolved IDBDatabase connection after its read or write operation completes
or fails.

services/loraAdapterService.ts (2)

129-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Collapse the two branches like decodeDatasetEntry does.

Both branches build the same object. decodeDatasetEntry (Line 426) and decodeTrainingRun (Line 553) already select the payload inline. Use the same shape here for consistency.

♻️ Proposed fix

-  if ('payload' in stored) {
-    const decoded = await readSecureRecordPayload<LoraAdapterMetaPayload>(stored.payload);
-    return {
-      meta: {
-        id: stored.id,
-        createdAt: stored.createdAt,
-        ...(stored.projectId !== undefined && { projectId: stored.projectId }),
-        ...decoded.value,
-      },
-      needsMigration: decoded.needsMigration,
-    };
-  }
-
-  const decoded = await readSecureRecordPayload<LoraAdapterMetaPayload>(adapterMetaPayload(stored));
+  const decoded = await readSecureRecordPayload<LoraAdapterMetaPayload>(
+    'payload' in stored ? stored.payload : adapterMetaPayload(stored),
+  );
   return {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/loraAdapterService.ts` around lines 129 - 155, Collapse the
duplicated branches in decodeAdapterMeta by selecting the secure payload inline,
using stored.payload when the stored value contains it and
adapterMetaPayload(stored) otherwise. Perform a single readSecureRecordPayload
call and retain the existing meta fields and needsMigration result.

---

86-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Also clear the cache when the connection closes unexpectedly.

onversionchange resets dbHandle and dbPromise. An unexpected close (browser eviction, forced close) fires onclose instead. In that case the cached handle stays, and every later transaction throws InvalidStateError for the lifetime of the page.

♻️ Proposed fix

       dbHandle.onversionchange = () => {
         dbHandle?.close();
         dbHandle = null;
         dbPromise = null;
       };
+      // QNBS-v3: Drop the cached handle on unexpected close so later calls can reopen the database.
+      dbHandle.onclose = () => {
+        dbHandle = null;
+        dbPromise = null;
+      };
       resolve(dbHandle);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/loraAdapterService.ts` around lines 86 - 101, Update the database
connection setup in the request success handler to assign an onclose callback
alongside onversionchange. When the connection closes unexpectedly, close or
clear the cached dbHandle and reset dbPromise so subsequent access reopens the
database instead of reusing an invalid handle.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes